[FSSDK-10265] fix: UPS Lookup & Save during batched Decide
#374
Conversation
I want this reference PR to be clear about what needs changing.
_decisionBatchInProgress from true to false
I'm hitting `Lookup` thrice for some reason in DecideAllWithUspShouldOnlyLookupSaveOnce
This reverts commit dfe1367.
mikechu-optimizely
changed the title
Lookup & Save during batched Decide
| private void SaveToUserProfileService(Experiment experiment = null, | ||
| Variation variation = null | ||
| ) |
There was a problem hiding this comment.
What about making experiment and variation NonNull and drop ?
I see this will be called only when a new decision is made.
| var userProfileMap = UserProfileService.Lookup(user.GetUserId()); | ||
| if (userProfileMap != null && | ||
| UserProfileUtil.IsValidUserProfileMap(userProfileMap)) | ||
| if (_userProfile == null) |
There was a problem hiding this comment.
For legacy, we need to load fresh _userProfile on every getVariation() call (even if _userProfile not null)
| @@ -1066,6 +1067,8 @@ OptimizelyDecideOption[] options | |||
| } | |||
| } | |||
|
|
|||
| DecisionService.DecisionBatchInProgress = false; | |||
There was a problem hiding this comment.
I see this will fire the saveUPS. Wondering if we have the similar fire solution for legacy APIs (activate, getVarition)
There was a problem hiding this comment.
I see this comment not pushed properly:
We probably need one more test -
decide(A)
decide(B)
2nd session picks up the UPS as expected
| userProfileServiceMock.Verify(l => l.Save(It.IsAny<Dictionary<string, object>>()), | ||
| Times.Once); | ||
| } | ||
|
|
There was a problem hiding this comment.
These tests look good. Wondering if we have tests covering the contents of the final UPS save call, combining all decisions in the session.
There was a problem hiding this comment.
Can we also have the similar coverage (lookup once - save once, the ups save contents) with legacy APIs (activate, getVariation) for regression?
There was a problem hiding this comment.
Added coverage of the final UPS Save as requested.
I'm researching the Activate and GetVariation tests where UPS is used.
Co-authored-by: Jae Kim <[email protected]>
…h-decide' into mike/FSSDK-10265-ups-during-batch-decide
Summary
LookupandSaveonce when usingDecideAllandDecideForKeys.Test plan
Issues